Skip to content

Comments

refactor: unify date filtering with column filter system#23773

Closed
eunjae-lee wants to merge 1 commit intofeat/date-target-insightsfrom
devin/date-column-filters-1757606055
Closed

refactor: unify date filtering with column filter system#23773
eunjae-lee wants to merge 1 commit intofeat/date-target-insightsfrom
devin/date-column-filters-1757606055

Conversation

@eunjae-lee
Copy link
Contributor

What does this PR do?

This PR refactors the insights date filtering system to unify date filtering with the existing column filter system, replacing the previous separate dateRange handling with dynamic column filters based on dateTarget ("startTime" or "createdAt").

Key Changes:

  • Replace hardcoded "timestamp" column with dynamic getDateColumn(dateTarget) function
  • Update backend to handle "startTime" and "createdAt" as date range column filters
  • Remove dateRange, startDate, endDate, and dateTarget from schemas and tRPC procedures
  • Extract date ranges from columnFilters instead of separate parameters
  • Unify date filtering with the general column filter system

Link to Devin run: https://app.devin.ai/sessions/6095c8602d414a0aabafe2eeaf5af329
Requested by: @eunjae-lee

This PR is stacked on top of #23752.

How should this be tested?

⚠️ Critical: This PR involves significant changes to date filtering logic and requires thorough testing:

  1. Date Filter Testing:

    • Test date filtering with both "startTime" and "createdAt" options
    • Verify date range selection works correctly for both targets
    • Test preset date ranges (Last 7 days, Last 30 days, etc.)
  2. Previous Period Comparisons:

    • Verify that booking stats show correct previous period comparisons
    • Test that percentage changes are calculated correctly
  3. URL State Persistence:

    • Change date ranges and verify URL updates correctly
    • Refresh page and verify filters persist
    • Test browser back/forward navigation
  4. CSV Export:

    • Test CSV export functionality with different date ranges
    • Verify filename includes correct dates
  5. Charts and Tables:

    • Verify all charts render correctly with filtered data
    • Test that data updates when changing date targets

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox. N/A - Internal refactoring with no user-facing changes.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

Human Review Checklist

⚠️ High Priority Items:

  • Test actual date filtering functionality - Verify both startTime and createdAt filtering work correctly with real data
  • Validate previous period calculations - Ensure booking stats comparisons still work after refactoring calculatePreviousPeriodDates
  • Check type safety - Review type assertions (as ColumnFilter) for potential runtime issues
  • Test error handling - Verify graceful handling when date ranges are missing from column filters
  • Validate CSV export - Ensure export functionality works with new date extraction logic
  • Test URL state persistence - Verify date filters persist correctly in URL and on page refresh

Medium Priority:

  • Review schema changes for potential breaking changes to other parts of the system
  • Verify all tRPC procedures correctly extract dates from column filters
  • Test edge cases like invalid date formats or missing date data

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked if my changes generate no new warnings

- Replace hardcoded timestampColumn with dynamic getDateColumn(dateTarget)
- Update useInsightsBookingParameters to use dateTarget for date range filtering
- Remove dateRange from backend service schema and add date handling to buildColumnFilterCondition
- Update ClearFiltersButton to exclude dynamic dateTarget instead of hardcoded 'timestamp'
- Extract date range from columnFilters in tRPC router instead of separate dateRange parameter
- Remove startDate, endDate, and dateTarget from base input schemas
- Add support for startTime and createdAt date column filtering in InsightsBookingBaseService
- Fix unused variable lint warning in trpc-router.ts

Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 11, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch devin/date-column-filters-1757606055

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@keithwillcode keithwillcode added consumer core area: core, team members only labels Sep 11, 2025
@eunjae-lee eunjae-lee closed this Sep 12, 2025
@keithwillcode keithwillcode deleted the devin/date-column-filters-1757606055 branch September 17, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consumer core area: core, team members only size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants